async_hooks,async_wrap: fixups and fewer aborts #14722

trevnorris · 2017-08-09T22:54:18Z

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

async_hooks, async_wrap

Commit summary:

Fix case where returning early from AsyncWrap::MakeCallback because of a disposed domain caused an abort.
Improvements to code comments and logic
Force exit less frequently

Note: Still reasoning about automatically coercing undefined values in a way that won't corrupt the stack.

CI: https://ci.nodejs.org/job/node-test-pull-request/9577/

AndreasMadsen

Only took a quick look.

AndreasMadsen · 2017-08-09T23:23:29Z

src/env-inl.h

@@ -127,8 +127,8 @@ inline v8::Local<v8::String> Environment::AsyncHooks::provider_string(int idx) {

 inline void Environment::AsyncHooks::push_ids(double async_id,
                                              double trigger_id) {
-  CHECK_GE(async_id, 0);
-  CHECK_GE(trigger_id, 0);
+  CHECK_GE(async_id, -1);


Why is -1 now a special allowed value?

AFAIK -1 is now used to represent a recoverable error in the graph.
https://github.com/trevnorris/node/blob/d21e3d9e7875ec8dfe478b587a932f7f3c56221c/lib/async_hooks.js#L381-L384
See the last comment in the OP Note: Still reasoning about automatically coercing undefined values in a way that won't corrupt the stack. the idea is to coerce all the places that exploded till now, because the asyncIDs where undefined, to -1 and treat those as "graph-opaque" bad actors instead of aborting.

I see. My personal opinion is that those checks should simply be skipped when AsyncHooks isn't used. Relaxing the check when AsyncHooks is used, isn't a good solution. I know that is an unpopular opinion, so I won't block it now, but I really feel we are on a steap slope to some very difficult to debug errors. In the future I want a more detailed discussion about this, unfortunately I don't have time these days.

from commit message body:

id values of -1 are allowed. They indicate that the id was never
correctly assigned to the async resource. These will appear in any
call graph, and will only be apparent to those using the async_hooks
module, then reported in an issue.

we can't guarantee the async id is always assigned properly, and was one reason applications were aborting. history has taught me that it's impossible to make sure the async id is always properly defined because of how much node internals can be abused. so -1 is to indicate that the async/trigger id wasn't properly set.

i'm working on one more commit to translate nan values to -1, but there are some edge case when tracking the stack that can leave the application in a bad state. if i can't figure those out soon will push it up regardless and look for feedback.

Ack.
I tend to agree, but the -1 trick currently isn't public API, so we could change it if we agree on a better solution.

@AndreasMadsen

My personal opinion is that those checks should simply be skipped when AsyncHooks isn't used.

It's not explicitly documented, but one goal of the API was to allow async_hooks.executionAsyncId() and async_hooks.triggerAsyncId() to be used regardless whether a hook has been enabled or not. We could possibly do the same trick that we do with domains and enable the checks if async_hooks is required, but domains is for performance reasons. Always performing these checks should be possible without ever causing the application to crash, and the additional cost probably isn't measurable.

Relaxing the check when AsyncHooks is used, isn't a good solution.

I fully agree'd with this until I realized how badly node core can be abused by allowing users to pass in arbitrary objects instead of ones created by core constructors (e.g. timers). So it is necessary to coerce, at minimum, async ids of undefined.

I really feel we are on a steap slope to some very difficult to debug errors.

Again, I fully agree'd with this until I came to terms with the reason pointed out above.

@refack

I tend to agree, but the -1 trick currently isn't public API, so we could change it if we agree on a better solution.

I was thinking about doing something more complex, like

-1 - indicates the async_id_symbol was undefined

-2 - indicates async_id_symbol was set but not a positive integer

etc. But figured that's too much to anticipate, and wouldn't be worth the time.

Please don't document it. I'm on vacation and of all the things I disagree with, our general approach to being robust against implementation errors is what I disagree the most about.

👍 on keeping this an "internal implementation" detail for now.

addaleax

PromiseHook() never needs to run domains

Why does it not need to do that?

addaleax · 2017-08-10T13:24:22Z

src/async-wrap.cc

@@ -696,7 +697,7 @@ MaybeLocal<Value> AsyncWrap::MakeCallback(const Local<Function> cb,
  }

  if (!PostCallbackExecution(this, true)) {
-    return Local<Value>();
+    return Undefined(env()->isolate());


Please also take a look at #14697 when you get the chance.

addaleax · 2017-08-10T13:34:59Z

lib/async_hooks.js

-      restoreTmpHooks();
+    // Hooks can only be restored if there have been no recursive hook calls.
+    // Also the active hooks do not need to be restored if enable()/disable()
+    // weren't called during hook execution. In which case


grammar nit: during hook execution, in which case

addaleax · 2017-08-10T13:35:22Z

lib/async_hooks.js

-    restoreTmpHooks();
+  // Hooks can only be restored if there have been no recursive hook calls.
+  // Also the active hooks do not need to be restored if enable()/disable()
+  // weren't called during hook execution. In which case


same grammar nit ;)

refack

CRs in tests
Other comments are nits/suggestions

refack · 2017-08-10T15:27:15Z

lib/async_hooks.js

-// Used to make sure active_hooks_array isn't altered in mid execution if
-// another hook is added or removed. A counter is used to track nested calls.
-var processing_hook = 0;
+// Use a counter to track nested calls of async hook callbacks and make sure


Suggestion:
since these four values are used to protect active_hooks_array's atomicity, we could group them into a "namespace":

const active_hooks = { array: [], call_depth: 0, tmp_array: null, tmp_fields: null, getHookArrays(), storeActiveHooks(), restoreActiveHooks() }

I like it.

EDIT: don't feel the need to have the functions in the same namespace since they're already defined outside of the script.

refack · 2017-08-10T15:32:51Z

lib/async_hooks.js

@@ -42,6 +54,8 @@ const { kInit, kBefore, kAfter, kDestroy, kTotals, kCurrentAsyncId,
        kCurrentTriggerId, kAsyncUidCntr,
        kInitTriggerId } = async_wrap.constants;

+// Symbols used to store the respective ids on both AsyncResource instances and
+// internal resources.


Is this more correct?
// Symbols used to store the respective ids on AsyncResource instances.
// Also used to cache the ids on internal resources for easy JS access.

yes, but should also add that they'll be added to any object that enters the internal async path; because users can pass arbitrary objects instead of using internally constructed ones.

EDIT: forgot the reason I left it at "internal resources" is because timers aren't AsyncResource instances and also don't read their asyncId from c++. this may also change in the future when the PR lands that allows arbitrary ids to be assigned to new AsyncWrap instances; which allows a JS object to receive an asyncId early.

refack · 2017-08-10T15:35:34Z

lib/async_hooks.js

@@ -53,6 +67,8 @@ const emitBeforeNative = emitHookFactory(before_symbol, 'emitBeforeNative');
 const emitAfterNative = emitHookFactory(after_symbol, 'emitAfterNative');
 const emitDestroyNative = emitHookFactory(destroy_symbol, 'emitDestroyNative');

+const abort_regex = /^--abort[_-]on[_-]uncaught[_-]exception$/;


could you add a TODO(refack): move to node-config.cc

refack · 2017-08-10T16:04:39Z

src/env-inl.h

@@ -127,8 +127,8 @@ inline v8::Local<v8::String> Environment::AsyncHooks::provider_string(int idx) {

 inline void Environment::AsyncHooks::push_ids(double async_id,
                                              double trigger_id) {
-  CHECK_GE(async_id, 0);
-  CHECK_GE(trigger_id, 0);
+  CHECK_GE(async_id, -1);


AFAIK -1 is now used to represent a recoverable error in the graph.
https://github.com/trevnorris/node/blob/d21e3d9e7875ec8dfe478b587a932f7f3c56221c/lib/async_hooks.js#L381-L384
See the last comment in the OP Note: Still reasoning about automatically coercing undefined values in a way that won't corrupt the stack. the idea is to coerce all the places that exploded till now, because the asyncIDs where undefined, to -1 and treat those as "graph-opaque" bad actors instead of aborting.

refack · 2017-08-10T16:23:00Z

test/parallel/test-domain-abort-when-disposed.js

+// When AsyncWrap::MakeCallback() returned an empty handle the
+// MaybeLocal::ToLocalChecked() call caused the process to abort. By returning
+// v8::Undefined() it allows the error to propagate to the 'error' event.
+s.on('error', common.mustCall((e) => {


What triggers this? the dispose on next tick?

The abort or the 'error' event?

The abort is because we're currently returning Local<Value>() from AsyncWrap::MakeCallback(); which now returns a MaybeLocal<Value>. So it will abort on MaybeLocal::ToLocalChecked() if the domain is disposed. Instead of propagating the error correctly.

the error event? since there is not interaction with s, I'm assuming that it's some cleanup code that triggers the error event.

@refack implementation details of how the JSStream instance dictates how the Socket connects. Because the JSStream instance isn't connected to anything the socket reports EINVAL.

refack · 2017-08-10T16:24:54Z

test/parallel/test-domain-abort-when-disposed.js

+'use strict';
+
+const common = require('../common');
+const JSStream = process.binding('js_stream').JSStream;


could you put this together with Socket and comment something like // arbitrary objects that trigger async_hooks

Sorry, not sure what you mean by "put this together with Socket". Mind clarifying?

const common = require('../common'); const assert = require('assert'); const domain = require('domain'); // These were picked arbitrarily and are only used to trigger arync_hooks const JSStream = process.binding('js_stream').JSStream; const Socket = require('net').Socket;

refack · 2017-08-10T16:26:15Z

lib/internal/errors.js

@@ -101,6 +101,8 @@ module.exports = exports = {
 // Note: Please try to keep these in alphabetical order
 E('ERR_ARG_NOT_ITERABLE', '%s must be iterable');
 E('ERR_ASSERTION', '%s');
+E('ERR_ASYNC_CALLBACK', (name) => `${name} must be a function`);


Could you add perfunctory tests in test/parallel/test-internal-errors.js as per https://github.com/nodejs/node/blob/master/doc/guides/using-internal-errors.md#testing-new-errors

trevnorris · 2017-08-11T06:53:35Z

@addaleax

PromiseHook() never needs to run domains

Why does it not need to do that?

I inferred that PromiseHook() never needed to run domains because it wasn't running domains in the previous code. I'd need to ask @matthewloring as to why it wasn't running domains before this.

trevnorris · 2017-08-11T07:36:20Z

@refack

the idea is to coerce all the places that exploded till now, because the asyncIDs where undefined, to -1 and treat those as "graph-opaque" bad actors instead of aborting.

One of the reasons for application crashing is because the async_id on the object was undefined, which necessitates properly coercing nan values internally. Brief list of the edge cases for handling this properly:

Setting async_id = -1 if async_id == nan in the before() call may become invalid if the instance's async_id value is set between the before() and after() calls. Technically invalidating the graph.
The async_id value may be set to -1 or nan if the instance is destroyed between the before() or after().

Simple way to handle this is to always succeed on the condition of either async_id or uid_fields_[kCurrentAsyncId] equaling -1. I believe this is a viable solution, but felt like I haven't worked though all the logic cases. Eh, I'll throw it up and run it through cigtm.

refack · 2017-08-11T11:33:33Z

PromiseHook() never needs to run domains

Why does it not need to do that?

I asked myself the same quastion and when I saw

node/src/async-wrap.cc

Line 375 in 66fd78e

PreCallbackExecution(wrap, false);

where the false argument means don't do domains.

v8::MaybeLocal::ToLocalChecked() will abort on an empty handle. AsyncWrap::MakeCallback returns an empty handle if the domain is disposed, but this should not cause the process to abort. So instead return v8::Undefined() and allow the error to be handled normally. PR-URL: nodejs#14722

* Reword several of the comments to be more descriptive. * Rename functions to better indicate what they are doing. * Change AsyncHooks::uid_fields_ to be a fixed array instead of a pointer. * Define regex early so line ends before 80 columns. * Remove obsolete comments. * Rename AsyncHooks::InitScope::uid_fields_ to uid_fields_ptr_ because using the same name as AsyncHooks::uid_fields_ was confusing. * Place variables that are used to store the new set of hooks if another hook is enabled/disabled during hook execution into an object to act as a namespace. PR-URL: nodejs#14722

* Because Emit{Before,After}() will always exit the process if there's an exception there's no need to check a return value. This simplifies the conditions and makes {Pre,Post}CallbackExecution() unnecessary. They have been removed and relevant code has been moved to the respective call sites. Which are: * PromiseHook() never needs to run domains, and without a return value to check place the calls to Emit{Before,After}() on location. * The logic in MakeCallback() is simplified by moving the single calls to Emit{Before,After}() then doing a simpler conditional to check if the domain has been disposed. * Change Domain{Enter,Exit}() to return true if the instance has been disposed. Makes the conditional check in MakeCallback() simpler to reason about. * Add UNREACHABLE() after FatalException() to make sure all error handlers have been cleared and the process really does exit. PR-URL: nodejs#14722

* id values of -1 are allowed. They indicate that the id was never correctly assigned to the async resource. These will appear in any call graph, and will only be apparent to those using the async_hooks module, then reported in an issue. * ids < -1 are still not allowed and will cause the application to exit the process; because there is no scenario where this should ever happen. * Add asyncId range checks to emitAfterScript(). * Fix emitBeforeScript() range checks which should have been || not &&. * Replace errors with entries in internal/errors. * Fix async_hooks tests that check for exceptions to match new internal/errors entries. NOTE: emit{Before,After,Destroy}() must continue to exit the process because in the case of an exception during hook execution the state of the application is unknowable. For example, an exception could cause a memory leak: const id_map = new Map(); before(id) { id_map.set(id, /* data object or similar */); }, after(id) { throw new Error('id never dies!'); id_map.delete(id); } Allowing a recoverable exception may also cause an abort because of a stack check in Environment::AsyncHooks::pop_ids() that verifies the async id and pop'd ids match. This case would be more difficult to debug than if fatalError() (lib/async_hooks.js) was called immediately. try { async_hooks.emitBefore(null, NaN); } catch (e) { } // do something async_hooks.emitAfter(5); It also allows an edge case where emitBefore() could be called twice and not have the pop_ids() CHECK fail: try { async_hooks.emitBefore(5, NaN); } catch (e) { } async_hooks.emitBefore(5); // do something async_hooks.emitAfter(5); There is the option of allowing mismatches in the stack and ignoring the check if no async hooks are enabled, but I don't believe going this far is necessary. PR-URL: nodejs#14722

addaleax · 2017-08-14T17:41:53Z

CI: https://ci.nodejs.org/job/node-test-commit/11760/

I intend to merge this if CI comes back good, but I agree that maybe this should not be the last word re: robustness of the checks.

addaleax · 2017-08-14T21:24:20Z

CI is green

Landed in 11a2ca2...062beb0

v8::MaybeLocal::ToLocalChecked() will abort on an empty handle. AsyncWrap::MakeCallback returns an empty handle if the domain is disposed, but this should not cause the process to abort. So instead return v8::Undefined() and allow the error to be handled normally. PR-URL: #14722 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Refael Ackermann <refack@gmail.com>

* Reword several of the comments to be more descriptive. * Rename functions to better indicate what they are doing. * Change AsyncHooks::uid_fields_ to be a fixed array instead of a pointer. * Define regex early so line ends before 80 columns. * Remove obsolete comments. * Rename AsyncHooks::InitScope::uid_fields_ to uid_fields_ptr_ because using the same name as AsyncHooks::uid_fields_ was confusing. * Place variables that are used to store the new set of hooks if another hook is enabled/disabled during hook execution into an object to act as a namespace. PR-URL: #14722 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Refael Ackermann <refack@gmail.com>

* Because Emit{Before,After}() will always exit the process if there's an exception there's no need to check a return value. This simplifies the conditions and makes {Pre,Post}CallbackExecution() unnecessary. They have been removed and relevant code has been moved to the respective call sites. Which are: * PromiseHook() never needs to run domains, and without a return value to check place the calls to Emit{Before,After}() on location. * The logic in MakeCallback() is simplified by moving the single calls to Emit{Before,After}() then doing a simpler conditional to check if the domain has been disposed. * Change Domain{Enter,Exit}() to return true if the instance has been disposed. Makes the conditional check in MakeCallback() simpler to reason about. * Add UNREACHABLE() after FatalException() to make sure all error handlers have been cleared and the process really does exit. PR-URL: #14722 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Refael Ackermann <refack@gmail.com>

* id values of -1 are allowed. They indicate that the id was never correctly assigned to the async resource. These will appear in any call graph, and will only be apparent to those using the async_hooks module, then reported in an issue. * ids < -1 are still not allowed and will cause the application to exit the process; because there is no scenario where this should ever happen. * Add asyncId range checks to emitAfterScript(). * Fix emitBeforeScript() range checks which should have been || not &&. * Replace errors with entries in internal/errors. * Fix async_hooks tests that check for exceptions to match new internal/errors entries. NOTE: emit{Before,After,Destroy}() must continue to exit the process because in the case of an exception during hook execution the state of the application is unknowable. For example, an exception could cause a memory leak: const id_map = new Map(); before(id) { id_map.set(id, /* data object or similar */); }, after(id) { throw new Error('id never dies!'); id_map.delete(id); } Allowing a recoverable exception may also cause an abort because of a stack check in Environment::AsyncHooks::pop_ids() that verifies the async id and pop'd ids match. This case would be more difficult to debug than if fatalError() (lib/async_hooks.js) was called immediately. try { async_hooks.emitBefore(null, NaN); } catch (e) { } // do something async_hooks.emitAfter(5); It also allows an edge case where emitBefore() could be called twice and not have the pop_ids() CHECK fail: try { async_hooks.emitBefore(5, NaN); } catch (e) { } async_hooks.emitBefore(5); // do something async_hooks.emitAfter(5); There is the option of allowing mismatches in the stack and ignoring the check if no async hooks are enabled, but I don't believe going this far is necessary. PR-URL: #14722 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Refael Ackermann <refack@gmail.com>

trevnorris · 2017-08-14T21:45:12Z

@addaleax Thanks for landing it.

I agree that maybe this should not be the last word re: robustness of the checks.

I am happy to entertain discussion on this topic. As the one who originally wrote all the checks, I fully believe the checks should be strict, but after several painful bugs and realizing node's API allows arbitrary objects to be passed in in-place-of internally constructed instances I realized that relaxing the checks was necessary (if in no other case when the async_id is undefined).